crypto: evict getCiphers/getHashes cache on setFips/setEngine#62985
crypto: evict getCiphers/getHashes cache on setFips/setEngine#62985srikanth-karthi wants to merge 4 commits intonodejs:mainfrom
Conversation
getCiphers() and getHashes() used cachedResult() which memoizes once and never clears. setFips() changes which algorithms OpenSSL exposes (FIPS-approved only vs. all), and setEngine() can register additional ciphers/hashes from a loaded engine, but neither invalidated the cache. Replace the two cachedResult() calls with manual cache variables (_ciphersCache, _hashesCache) that mirror the existing _hashCache pattern. Add evictCipherHashCache() and call it from both setFips() and setEngine() after they mutate OpenSSL state. getCurves is intentionally left using cachedResult — curves are not affected by FIPS mode or engine loading. Fixes: nodejs#62982
|
Review requested:
|
nodejs-github-bot
added
crypto
|
@srikanth-karthi thank you, this looks good on first a cursory look but can you add a test? we do have fips enabled runners in CI that could use one. |
|
As #62982 details mention: under BoringSSL, internal code in node/lib/internal/crypto/util.js Lines 405 to 410 in bb85d23 Those results are also stale |
|
BoringSSL has no fips mode. And when it's not boringssl and the ciphers "become unavailable" due to a toggle switch they'll start failing with The "fips to non-fips" case 🤷 Imho not worth the churn. |
|
Thanks for the context. Agreed — under BoringSSL there's no FIPS mode so conditionalAlgorithms is stable for the process lifetime, and for OpenSSL the SubtleCrypto.supports() inconsistency after a FIPS toggle is an accepted limitation given the churn required to fix it. I'll leave conditionalAlgorithms out of scope for this PR. |
panva
added
the
commit-queue-squash
|
@ChALkeR what do you think about keeping the identity of the cached result the same? that way consumer can actually rely on their returned value and not get tripped up by someone elses' concern flipping fips mode. |
Add a FIPS-only regression test for nodejs#62982 that confirms getCiphers() and getHashes() reflect the restricted FIPS algorithm set after setFips(true) and restore the full list after setFips(false). Signed-off-by: srikanth-karthi <[email protected]>
Capture initialFips state and explicitly disable FIPS before recording the baseline algorithm lists, so the test works correctly on systems where FIPS is enabled by default. Restore the original state on exit. Signed-off-by: srikanth-karthi <[email protected]>
Each test runs in its own process so restoring the initial FIPS state is unnecessary. Signed-off-by: srikanth-karthi <[email protected]>
srikanth-karthi
force-pushed
the
fix-cipher-hash-cache-eviction
branch
from
967e31f to
76e61cb
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #62985 +/- ##
=======================================
Coverage 89.66% 89.66%
=======================================
Files 707 707
Lines 219501 219523 +22
Branches 42087 42092 +5
=======================================
+ Hits 196807 196832 +25
Misses 14588 14588
+ Partials 8106 8103 -3
🚀 New features to boost your workflow:
|
4 participants
Fixes: #62982
Summary
getCiphers()andgetHashes()usedcachedResult()which memoizes on first call and never clears. Two operations mutate the OpenSSL algorithm set but did not invalidate the cache:setFips(true/false)— restricts/restores the visible cipher/hash list to FIPS-approved algorithms onlysetEngine(id, flags)— loads an OpenSSL engine that may register additional ciphers/hashesThis meant that after calling either function,
getCiphers()andgetHashes()would continue returning the stale pre-call snapshot.Changes
cachedResult()calls forgetCiphers/getHashesinlib/internal/crypto/util.jswith manuallet _ciphersCache/let _hashesCachevariables — mirroring the existing_hashCache/getHashCache()pattern already in the same file.evictCipherHashCache()that resets both variables toundefined.evictCipherHashCache()fromsetEngine()(after a successful engine load) and fromsetFips()(aftersetFipsCrypto(val)).getCurvesintentionally keeps usingcachedResult()— curves are not affected by FIPS mode or engine loading.